python side PR

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @TzahiTaub)

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

                        !subtree.is_unmodified(),
                        "Unexpectedly deserialized leaf sibling."
                    );

I understand why this would be unexpected, but is it an unrecoverable error?

Code quote:

                    assert!(
                        !subtree.is_unmodified(),
                        "Unexpectedly deserialized leaf sibling."
                    );

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

                    if let Some(ref mut leaves) = previous_leaves {
                        leaves.insert(subtree.root_index, previous_leaf);
                    }

is the else case here an unrecoverable error?
if so please add a TODO to panic here on else (maybe even just call expect on previous_leaves)

Code quote:

                    if let Some(ref mut leaves) = previous_leaves {
                        leaves.insert(subtree.root_index, previous_leaf);
                    }

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 347 at r1 (raw file):

        should_fetch_modified_leaves: bool,
    ) {
        if !subtree.is_leaf() || (should_fetch_modified_leaves && !subtree.is_unmodified()) {

can you explain what is the logical change here?

Code quote:

if !subtree.is_leaf() || (should_fetch_modified_leaves && !subtree.is_unmodified()) {

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

It's not an error, for storage trees without the trivial modifications flag, we don't want the previous leaves of modifications. It's only an error if config.compare_modified_leaves() is True right? Maybe we should assert this at the beginning of the function.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

OK, so not a business logic error anyway? I wouldn't want this config to cause new panics, so I'm unblocking

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

I added this as a sanity check to make sure that we don't deserialize a leaf sibling...
We assume that leaf siblings do not exist in storage, so if we actually get a leaf sibling, we can't know if the deserialization has been done correctly.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

not exactly.
leaves is not none in the case that we want also to return the previous leaves at the modified indices. It's independent of config.compare_modified_leaves() configuration.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 347 at r1 (raw file):

Now we no longer assume that we can deserialize leaf siblings. (they do not exist in storage)
This function decides whether we should deserialize the next subtree or not.
If it's an inner node, we always have to deserialize.
If it's a leaf sibling, we don't really need (and can't - since we don't have it in storage) - the hash is enough.
If it's a modified leaf, then it depends on should_fetch_modified_leaves flag.

The change is that previously we deserialized a leaf sibling iff we deserialized a modified leaf.
Now, in any case, we don't deserialize a leaf sibling.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

so if we actually get a leaf sibling, we can't know if the deserialization has been done correctly.

how can we incorrectly deserialize a leaf?
if it's not in storage we will get a missing key error from storage (in the future, everything should exist in storage anyway, just a matter of deciding to read or not);
but if we don't get a missing key error, how will the deserialization be "done incorrectly"?

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 217 at r1 (raw file):

                        log_trivial_modification!(subtree.root_index, previous_leaf);
                    }
                    if let Some(ref mut leaves) = previous_leaves {

is this comment correct? if so please add it

Suggestion:

                    }

                    // If previous values of modified leaves are required, add this leaf.
                    if let Some(ref mut leaves) = previous_leaves {

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 217 at r1 (raw file):

required requested

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

you are right.
i added this assert to make sure that if we deserialize a leaf it's not a sibling. do you want me to remove this check?

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 217 at r1 (raw file):

Done.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

keep the check but just log a warning if the subtree is modified

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

It's not indeoendent. If config.compare_modified_leaves() is True, then previous_leaves shouldn't be None in any case (and if it is, it's an error), doesn't it? If it's False, then it should be None for one Tree and False for another.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

Let's take the storage tries for example: config.compare_modified_leaves() is True, but previous_leaves is none.
the variable previous_leaves is used and returned when create_and_get_previous_leaves is called

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

Done.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)